Cleanup, Tests | MultipartIdentifier by edwardneal · Pull Request #3891 · dotnet/SqlClient

edwardneal · 2026-01-14T06:57:42Z

Description

This work performs a small cleanup of the MultipartIdentifier type, and moves its test into the UnitTests project. That simplifies some of the tech debt identified in #3890, since we can use InternalsVisibleTo to access the type directly rather than manually include the source file.

This can be reviewed commit-by-commit - I don't expect any behavioural changes. There's existing test coverage, and this passes, but everything can be verified statically. These changes essentially remove parameters which are always constant, but which were passed as a result of the MultipartIdentifier type being originally used for providers other than SqlClient.

Issues

None, but dovetails with #3890 - @benrr101 for awareness.

Testing

Existing automatic tests cover this.

These were always passed with the same, constant, value.

This tested a case which would never occur.

Also remove now-unnecessary test case.

Also remove a now-unnecessary test case.

Remove unnecessary IsWhitespace method. Replace always-true runtime checks against constant values with debug assertions.

This tested the exception thrown when limit is zero. This is always either 3 or 4.

…ional inclusion of MultipartIdentifier from FunctionalTests

paulmedynski · 2026-01-14T12:30:09Z

/azp run

azure-pipelines · 2026-01-14T12:30:32Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski

Another good move of some true unit tests, and valuable cleanup along the way. A few suggestions to improve the cleanup even more.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/MultipartIdentifier.cs

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/Common/MultipartIdentifierTests.cs

Eliminates some ambiguity between "name" and "identifier", "identifier part" or "property name".

edwardneal · 2026-01-14T23:23:19Z

Thanks @paulmedynski - I've just replied to the first round of review comments.

github-actions · 2026-02-14T02:55:15Z

This pull request has been marked as stale due to inactivity for more than 30 days.

If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days.

edwardneal · 2026-02-14T13:42:32Z

This PR is not stale.

paulmedynski · 2026-02-17T12:01:49Z

@edwardneal Can you address the conflicts, and then I'll kick the pipelines and re-review.

edwardneal · 2026-02-17T18:18:40Z

Thanks @paulmedynski - conflicts are merged.

paulmedynski · 2026-02-18T18:21:17Z

/azp run

azure-pipelines · 2026-02-18T18:21:43Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski

Great improvements, now looking for some wider unit test coverage

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/Common/MultipartIdentifierTests.cs

Wraith2 · 2026-02-18T23:27:07Z

A few years ago I rewrote the multipart identifier class so that it no longer requires that it return string[] but can use a span or memory to contain the parse information and you can then extract the parts of the string that you want and write them to a stream or string as you need to. It looks like I never got around to PR'ing because there were more important things with higher impact in progress. Is there interest in a version which has lower possible allocations?

Replace various test cases with a Theory-based approach. Widen code coverage, including cases for [], " and whitespace. Prove both cases for throwOnEmptyMultiPartIdentifier.

paulmedynski · 2026-03-09T13:53:18Z

/azp run

azure-pipelines · 2026-03-09T13:53:45Z

Azure Pipelines successfully started running 2 pipeline(s).

codecov · 2026-03-09T14:58:18Z

Codecov Report

❌ Patch coverage is 98.73418% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.75%. Comparing base (0208d81) to head (1e28086).
⚠️ Report is 28 commits behind head on main.

Files with missing lines	Patch %	Lines
...lClient/src/Microsoft/Data/SqlClient/SqlCommand.cs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           main    #3891       +/-   ##
=========================================
+ Coverage      0   64.75%   +64.75%     
=========================================
  Files         0      282      +282     
  Lines         0    66045    +66045     
=========================================
+ Hits          0    42765    +42765     
- Misses        0    23280    +23280

Flag	Coverage Δ
PR-SqlClient-Project	`64.75% <98.73%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

paulmedynski · 2026-03-09T17:32:23Z

@Wraith2 - Yes, please open a PR with your changes!

edwardneal added 14 commits January 13, 2026 23:09

Remove leftQuote and rightQuote parameters

65b528e

These were always passed with the same, constant, value.

Remove constant-value separator parameter

62988ae

Remove constant-value removeQuotes parameter

3f12787

Remove unnecessary test case

b7f564a

This tested a case which would never occur.

Minor comment cleanup

5d084e3

Remove constant-value rightQuote parameter

f797d3b

Also remove now-unnecessary test case.

Remove constant-value leftQuote parameter

ae5551b

Also remove a now-unnecessary test case.

Minor cleanup

2215652

Remove unnecessary IsWhitespace method. Replace always-true runtime checks against constant values with debug assertions.

Comment and syntax cleanup

df55f47

if-block cleanup

4f6bfd4

Remove unnecessary test case

daa76c3

This tested the exception thrown when limit is zero. This is always either 3 or 4.

Move MultipartIdentifierTests to the UnitTests project, remove condit…

8830d5c

…ional inclusion of MultipartIdentifier from FunctionalTests

Cleanup warning in MultipartIdentifierTests

5c581fb

Remove now-unnecessary ParseMultipartIdentifier overload

c79ac4b

edwardneal requested a review from a team as a code owner January 14, 2026 06:57

github-project-automation bot added this to SqlClient Board Jan 14, 2026

github-project-automation bot moved this to To triage in SqlClient Board Jan 14, 2026

paulmedynski moved this from To triage to In review in SqlClient Board Jan 14, 2026

paulmedynski added the Area\Tests Issues that are targeted to tests or test projects label Jan 14, 2026

paulmedynski requested changes Jan 14, 2026

View reviewed changes

github-project-automation bot moved this from In review to In progress in SqlClient Board Jan 14, 2026

edwardneal added 4 commits January 14, 2026 22:39

Move debug assertions to comments on the constants

36239ad

Naming cleanup

149606f

Eliminates some ambiguity between "name" and "identifier", "identifier part" or "property name".

Enable nullability annotations

1f7cde9

Sense-check names of tests

1b5acff

github-actions bot added the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Feb 14, 2026

github-actions bot removed the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Feb 15, 2026

Merge main

95f7b17

paulmedynski requested changes Feb 18, 2026

View reviewed changes

paulmedynski self-assigned this Feb 18, 2026

edwardneal added 2 commits February 21, 2026 19:06

Merge branch 'main' into cleanup/multipartidentifier

c35c9a2

Respond to code review

1e28086

Replace various test cases with a Theory-based approach. Widen code coverage, including cases for [], " and whitespace. Prove both cases for throwOnEmptyMultiPartIdentifier.

paulmedynski added this to the 7.0.0 milestone Mar 9, 2026

paulmedynski approved these changes Mar 9, 2026

View reviewed changes

cheenamalhotra approved these changes Mar 9, 2026

View reviewed changes

paulmedynski merged commit 326a242 into dotnet:main Mar 9, 2026
296 checks passed

github-project-automation bot moved this from In progress to Done in SqlClient Board Mar 9, 2026

edwardneal deleted the cleanup/multipartidentifier branch March 9, 2026 19:32

Conversation

edwardneal commented Jan 14, 2026

Description

Issues

Testing

Uh oh!

paulmedynski commented Jan 14, 2026

Uh oh!

azure-pipelines bot commented Jan 14, 2026

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edwardneal commented Jan 14, 2026

Uh oh!

github-actions bot commented Feb 14, 2026

Uh oh!

edwardneal commented Feb 14, 2026

Uh oh!

paulmedynski commented Feb 17, 2026

Uh oh!

edwardneal commented Feb 17, 2026

Uh oh!

paulmedynski commented Feb 18, 2026

Uh oh!

azure-pipelines bot commented Feb 18, 2026

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Wraith2 commented Feb 18, 2026

Uh oh!

paulmedynski commented Mar 9, 2026

Uh oh!

azure-pipelines bot commented Mar 9, 2026

Uh oh!

codecov bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

paulmedynski commented Mar 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 9, 2026 •

edited

Loading